Skip to content

[composable-controller] Subscribe to stateChange events of V1 controllers with messenger, type fixes#3964

Merged
MajorLift merged 12 commits intomainfrom
240222-composable-controller-type-fixes
Mar 7, 2024
Merged

[composable-controller] Subscribe to stateChange events of V1 controllers with messenger, type fixes#3964
MajorLift merged 12 commits intomainfrom
240222-composable-controller-type-fixes

Conversation

@MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Feb 23, 2024 •
edited
Loading

Explanation

  • If a controller is V1 but it has a messaging system, subscribe to its stateChange event.
  • Remove @metamask/utils as a dependency.
  • Remove package-level exports for BaseControllerV{1,2}Instance, ControllerInstance, as these are incomplete and unsuited for general usage. isBaseController{,V1} are not removed.
    • Rename BaseControllerV2Instance as BaseControllerInstance for consistency with isBaseController.

References

Changelog

@metamask/composable-controller

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@MajorLift MajorLift added the team-wallet-framework Deprecated: Please use `team-core-platform` instead. label Feb 23, 2024
@MajorLift MajorLift self-assigned this Feb 23, 2024
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from cac2559 to 0c830ba Compare February 23, 2024 04:39
@MajorLift MajorLift linked an issue Feb 23, 2024 that may be closed by this pull request
[composable-controller] Fix incorrect #updateChildController behavior and improve typing #3907
Closed
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from 0c830ba to 599fd44 Compare February 23, 2024 04:48
@MajorLift MajorLift removed a link to an issue Feb 23, 2024
[composable-controller] Fix incorrect #updateChildController behavior and improve typing #3907
Closed
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from bdc167c to 0882483 Compare February 23, 2024 19:30
@MajorLift MajorLift changed the title [composable-controller] Type fixes, cleanup [composable-controller] Subscribe to stateChange events of V1 controllers with messenger, type fixes Feb 23, 2024
@MajorLift MajorLift marked this pull request as ready for review February 23, 2024 19:37
@MajorLift MajorLift requested a review from a team as a code owner February 23, 2024 19:37
@MajorLift MajorLift linked an issue Feb 23, 2024 that may be closed by this pull request
[composable-controller] Subscribe to stateChange events of V1 controllers with messenger #3966
Closed
@cryptodev-2s
Copy link
Contributor

cryptodev-2s commented Feb 26, 2024

@MajorLift in case of BaseControllerV1 without messagingSystem shouldn't we throw an error ? Or in this case I guess we would want only to save the initial state within the ComposableController ?
Other ways you PR looks good

@MajorLift
Copy link
Contributor Author

MajorLift commented Feb 26, 2024

@cryptodev-2s Hey thanks for taking a look at this! A BaseControllerV1 controller without a messagingSystem would be the default expectation and not an error case, since the controller-messenger pattern was only introduced in BaseControllerV2.

This PR enables composable-controller to handle V1 controllers like NftController, TokensController, TransactionsController that do have a messagingSystem because they are halfway there to upgrading to V2, but for regular V1 controllers where that's not the case, composable-controller's existing logic for merging state and subscribing a listener to the controller is preserved.

cryptodev-2s
cryptodev-2s previously approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from 6ef730e to 0fa97c3 Compare February 27, 2024 23:33
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from 0ab242a to 0fa97c3 Compare March 6, 2024 16:34
Base automatically changed from 240221-base-controller-type-fixes to main March 7, 2024 18:24
@MajorLift MajorLift dismissed cryptodev-2s’s stale review March 7, 2024 18:24

The base branch was changed.

@MajorLift MajorLift requested a review from a team as a code owner March 7, 2024 18:24
MajorLift added 9 commits March 7, 2024 13:25
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from 0fa97c3 to 7d46b06 Compare March 7, 2024 18:26
@MajorLift MajorLift removed the request for review from a team March 7, 2024 18:36
mcmire
mcmire reviewed Mar 7, 2024
View reviewed changes
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good! Just had one question.

packages/composable-controller/src/ComposableController.ts
*
* The `BaseController` class itself can't be used directly as a type representing all of its subclasses,
* because the generic parameters it expects require knowing the exact shape of the controller's state and messenger.
* Note that this type is not the greatest subtype or narrowest supertype of all `BaseController` instances.
Copy link
Contributor

@mcmire mcmire Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

packages/composable-controller/src/ComposableController.ts
(isBaseControllerV1(controller) && 'messagingSystem' in controller) ||
isBaseController(controller)
) {
this.messagingSystem.subscribe(
Copy link
Contributor

@mcmire mcmire Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a BaseControllerV1 controller has a messagingSystem property, could the state of this controller could be updated twice per update? If so, do we still need an else if?

Copy link
Contributor Author

@MajorLift MajorLift Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point! Fixed here: 811a94a

MajorLift and others added 2 commits March 7, 2024 16:13
mcmire
mcmire approved these changes Mar 7, 2024
View reviewed changes
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@MajorLift MajorLift merged commit 964e389 into main Mar 7, 2024
@MajorLift MajorLift deleted the 240222-composable-controller-type-fixes branch March 7, 2024 21:24
@MajorLift MajorLift mentioned this pull request Apr 25, 2024
Merged
3 tasks
@MajorLift MajorLift mentioned this pull request Jun 15, 2024
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcmire mcmire mcmire approved these changes

@cryptodev-2s cryptodev-2s cryptodev-2s left review comments

Assignees

@MajorLift MajorLift

Labels

base-controller-breaking-changes team-wallet-framework Deprecated: Please use `team-core-platform` instead.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[composable-controller] Subscribe to stateChange events of V1 controllers with messenger

3 participants

@MajorLift @cryptodev-2s @mcmire